Skip to content

chore: remove tests for private tenuous zarr-python store methods#2175

Closed
d-v-b wants to merge 7 commits into
earth-mover:mainfrom
d-v-b:remove-store-private-method-tests
Closed

chore: remove tests for private tenuous zarr-python store methods#2175
d-v-b wants to merge 7 commits into
earth-mover:mainfrom
d-v-b:remove-store-private-method-tests

Conversation

@d-v-b

@d-v-b d-v-b commented Jun 3, 2026

Copy link
Copy Markdown

I couldn't run tests locally because compilation failed during linking with cc, but I bet CI will be sufficient for this contribution.

@dcherian
dcherian enabled auto-merge June 3, 2026 21:24
@ianhi

ianhi commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Hi Davis thanks for this. Can you explain the motivation and what you mean by "tenuous" here? These overrides appear to have been doing something useful as without them we get these failures:

FAILED tests/test_zarr/test_store/test_icechunk_store.py::TestIcechunkStore::test_get_bytes[1] - icechunk.IcechunkError: x bad metadata

@d-v-b

d-v-b commented Jun 23, 2026

Copy link
Copy Markdown
Author

Can you explain the motivation and what you mean by "tenuous" here?

The methods being tested in the code I excise here were removed from zarr-python recently.

@d-v-b

d-v-b commented Jun 23, 2026

Copy link
Copy Markdown
Author

haha I never scrolled all the way up to see that these are overrides for the store tests we ship in zarr-python! this PR should be amended to gate on the zarr-python version instead of just removing these overrides!

@dcherian

Copy link
Copy Markdown
Collaborator

IMO these tests all suck, both in IC and in Zarr. They should be property tests and should be in the public testing. api so that external stores can run them.

@d-v-b

d-v-b commented Jun 24, 2026

Copy link
Copy Markdown
Author

IMO these tests all suck, both in IC and in Zarr. They should be property tests and should be in the public testing. api so that external stores can run them.

100%, these tests are no good. And we should support icechunk's needs by supporting test configuration (e.g., choosing the stored values to narrowly contain zarr metadata docs)

@dcherian

Copy link
Copy Markdown
Collaborator

hah forgot that complaint, they should be generating keys that are expected from a reading of the spec!

@d-v-b

d-v-b commented Jun 24, 2026

Copy link
Copy Markdown
Author

hah forgot that complaint, they should be generating keys that are expected from a reading of the spec!

that's complicated! we need to support zarr v2, which includes consolidated metadata (.zmetadata keys) which is not in any spec, and zarr v3's consolidated metadata (spec drafted, but stalled over in zarr-specs) reserves the 'kind field for future use, anticipating storing consolidated metadata in some external key with an unknown name. And then there are chunk key encodings...

@dcherian

Copy link
Copy Markdown
Collaborator

sure... but there's still a small subset of patterns not foo.json with crap in it.

li-em added a commit that referenced this pull request Jul 24, 2026
li-em added a commit that referenced this pull request Jul 24, 2026
li-em added a commit that referenced this pull request Jul 24, 2026
@li-em

li-em commented Jul 24, 2026

Copy link
Copy Markdown
Member

Fixed in #2294 by checking for the private attributes instead, while we don't bump the minimum supported version to one that includes the removal in zarr

@li-em li-em closed this Jul 24, 2026
auto-merge was automatically disabled July 24, 2026 21:57

Pull request was closed

DahnJ pushed a commit to DahnJ/icechunk that referenced this pull request Jul 24, 2026
…eam GHA (earth-mover#2294)

Couple of different problems detected in earth-mover#1921 fixed on this PR:

-
[scientific-python/issue-from-pytest-log-action](scientific-python/issue-from-pytest-log-action#74)
was not uploading the issue body anymore, fixed upstream, bumped version
here.

- earth-mover#2175 removed our zarr tests overrides, but that leaves us in an
inconsistent state while zarr 3.3 (or the current `main`) is not
released, and we bump our minimum supported zarr. Opted to checking if
the `_get_*` exist instead of a version range check, and we can actually
drop the overrides once we move newer zarr releases

## stateful testing fixes

- `test_zarr/test_stateful.py`: use the storage-key chunk grid in
`shift_array`. The rule used `cdata_shape`, which counts *inner* chunks
for sharded arrays, while store keys and `Session.shift_array` operate
on the outer (shard) grid. zarr main's hypothesis strategies now
generate sharded arrays, exposing this: the model shuffled keys over a
grid that doesn't exist (e.g. producing `0/c/1` for a single-shard
array). A new `storage_chunk_sizes` helper uses zarr's
`write_chunk_sizes` when available and derives the grid from `shards or
chunks` on older zarr. Includes deterministic regression tests, verified
against both zarr 3.1.6 and zarr nightly.

- `test_stateful_repo_ops.py`: match the model's GC re-parenting to
Rust. The model re-parented kept snapshots whose parent was GC'd to the
*initial* snapshot, but `delete_snapshots_from_repo_info` re-parents
over the deleted run to the *nearest surviving ancestor*. After a
`reset_branch` back onto such a snapshot, model and repo disagreed on
reachability, and the next `garbage_collect` predicted the wrong
`snapshots_deleted` count. Triggered only when a GC deletes a middle run
while older (ref-pointed) and newer (`flushed_at`-protected) snapshots
both survive — hence the intermittent nightly appearances.

- `test_stateful_repo_ops.py`: guard `create_branch`/`create_tag`
against V1 expired-ancestor chains. V1 expiration rewrites only
ref-reachable chains, so an unreachable commit's on-disk ancestry can
still pass through expired snapshots the model dropped; creating a ref
there resurrects history the model no longer tracks and fails
`check_ancestry`. Applies the same full-ancestry `assume()` that
`reset_branch` already uses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants